unified: Build instance-member namespace and resolve unqualified lookups - #22420
Open
asgerf wants to merge 12 commits into
Open
unified: Build instance-member namespace and resolve unqualified lookups#22420asgerf wants to merge 12 commits into
asgerf wants to merge 12 commits into
Conversation
I thought there was a bug here, but it turns out this works as intended. Makes sense to keep the test though.
Originally this node was intended to cover TopLevels as well, but things evolved differently, and this name works better since it's only for classes.
Avoids surprising edges cases when a class has no instance/static members
UnqualifiedMemberAccess might go into Public one day
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * `instanceAccess` indicates if this this member should be accessed as an instance of `accessingClass` | ||
| * or as a static member. | ||
| */ | ||
| private predicate unqualifiedMemberAccessCand( |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds instance-member namespaces and shadowing-aware unqualified name resolution to unified Swift analysis.
Changes:
- Models static and instance namespaces, including inheritance.
- Exposes unqualified member bindings through definitions and tests.
- Refines static name-resolution statistics.
Show a summary per file
| File | Description |
|---|---|
unified/ql/test/library-tests/static-name-binding/unqualified-access.swift |
Updates inherited lookup expectations. |
unified/ql/test/library-tests/static-name-binding/test.swift |
Adds enum-case binding coverage. |
unified/ql/test/library-tests/static-name-binding/test.ql |
Tests the new binding API. |
unified/ql/test/library-tests/static-name-binding/implicit-instance-field-access.swift |
Tests implicit instance-member access. |
unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll |
Implements namespaces and unqualified lookup. |
unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll |
Defines Swift inheritance behavior. |
unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll |
Adds member classification extension points. |
unified/ql/lib/codeql/unified/internal/FacadeAst.qll |
Adds AST depth calculation. |
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll |
Refines resolution statistics. |
unified/ql/lib/codeql/Definitions.qll |
Uses shadowing-aware bindings for definitions. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll:623
- Use “an” before “unqualified.”
* An identifier appearing in a unqualified position, referring to a member of an enclosing class.
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll:36
- Remove the extraneous article in this sentence.
// Base expression is a confirmed to depend on type inference
- Files reviewed: 10/10 changed files
- Comments generated: 8
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
hvitved
reviewed
Aug 27, 2026
hvitved
left a comment
hvitved
left a comment
Contributor
There was a problem hiding this comment.
Looks good, thanks for making the code more easily reviewable via multiple commit.
| node1.isIdentifier(nameDecl) and | ||
| name = nameDecl.getName() and | ||
| node2.isStaticMemberNamespace(cls) | ||
| ( |
Contributor
There was a problem hiding this comment.
I don't think the parens are needed here.
| /** Holds if this namespace has an own-member of the given name */ | ||
| predicate hasOwnMember(string name) { exists(this.getOwnMember(name)) } | ||
|
|
||
| /** If this is the static namespace for a class, gets the corresponding instance namespace. */ |
| ) | ||
| } | ||
|
|
||
| /** If this is the instance namespace for a class, gets the corresponding static namespace. */ |
| /** A name-binding node that has members. */ | ||
| class NamespaceNode extends NameBindingNode { | ||
| NamespaceNode() { storeStep(_, _, this) or inheritanceStep(_, this) } | ||
| NamespaceNode() { |
Contributor
There was a problem hiding this comment.
Perhaps the QL doc should say that can have members.
| ) | ||
| } | ||
|
|
||
| /** Holds if the member represented by `node` cannot be inherited. */ |
Contributor
There was a problem hiding this comment.
I see this was fixed in a later commit.
| } | ||
|
|
||
| private int unqualifiedMemberAccessDepth(PotentialLocalNameAccess access) { | ||
| result = max(AstNode scope | unqualifiedMemberAccessCand(access, _, _, scope) | scope.getDepth()) |
Contributor
There was a problem hiding this comment.
Is this part only needed when looking up in uncertain scopes? I.e., if we can resolve by local scoping (the first case in unqualifiedMemberAccessCand), will that then always be the answer?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does a few things:
selfaccess).UnqualifiedMemberAccessclass, which should provide the needed information to consumers (type inference and data flow), without leaking internal details about name-binding nodes.xinfoo().x.